Skip to content

Support cross-locale slug lookup in language switcher #6634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+166 −20
Open

Support cross-locale slug lookup in language switcher #6634

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
154cfbc
Support cross-locale slug lookup in language switcher
smithellis Apr 22, 2025
9660098
improve tests
smithellis Apr 23, 2025
c46afbd
decrease comments and refactor get_visible_document_or_404
smithellis May 5, 2025
ca2836a
refactor loop and cut down on comments
smithellis May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions kitsune/wiki/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@
generate_short_url,
get_featured_articles,
get_kb_visited,
get_visible_document_or_404,
has_visited_kb,
num_active_contributors,
remove_expired_from_kb_visited,
update_kb_visited,
)
from django.http import Http404


class ActiveContributorsTestCase(TestCase):
Expand Down Expand Up @@ -700,3 +702,135 @@ def test_with_product_and_topic_2(self):
},
)
self.assertFalse(self.session.modified)


class GetVisibleDocumentTests(TestCase):
def setUp(self):
self.user = UserFactory()

self.en_doc = ApprovedRevisionFactory(
document__locale="en-US",
document__slug="test-document",
is_ready_for_localization=True,
).document

self.de_rev = TranslatedRevisionFactory(
document__locale="de",
document__slug="test-document-de",
document__parent=self.en_doc,
based_on=self.en_doc.current_revision,
)
self.de_doc = self.de_rev.document

self.fr_rev = TranslatedRevisionFactory(
document__locale="fr",
document__slug="test-document-fr",
document__parent=self.en_doc,
based_on=self.en_doc.current_revision,
)
self.fr_doc = self.fr_rev.document

def test_get_document_by_locale_and_slug_direct_match(self):
"""Test getting a document when there's a direct match for locale and slug."""
doc = get_visible_document_or_404(self.user, locale="en-US", slug="test-document")
self.assertEqual(doc, self.en_doc)

doc = get_visible_document_or_404(self.user, locale="de", slug="test-document-de")
self.assertEqual(doc, self.de_doc)

doc = get_visible_document_or_404(self.user, locale="fr", slug="test-document-fr")
self.assertEqual(doc, self.fr_doc)

def test_get_document_via_translation_for_nondefault_locale(self):
"""Test getting a document via its parent when no direct match in the requested locale."""
with self.assertRaises(Http404):
# This should raise a 404 because we're explicitly
# setting look_for_translation_via_parent=False
get_visible_document_or_404(
self.user,
locale="en-US",
slug="test-document-de",
look_for_translation_via_parent=False,
)

# This should also raise a 404 because cross-locale lookups should only work when
# look_for_translation_via_parent is True
with self.assertRaises(Http404):
get_visible_document_or_404(self.user, locale="en-US", slug="test-document-de")

def test_cross_locale_lookup(self):
"""Test the new cross-locale lookup functionality for language switcher."""
doc = get_visible_document_or_404(
self.user,
locale="en-US",
slug="test-document-de",
look_for_translation_via_parent=True,
)
self.assertEqual(doc, self.en_doc)

doc = get_visible_document_or_404(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same assertion with the German document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow you - all these tests have subtle differences, either slug or locale or other flags. Let me know if I've missed something.

self.user,
locale="en-US",
slug="test-document-fr",
look_for_translation_via_parent=True,
)
self.assertEqual(doc, self.en_doc)

doc = get_visible_document_or_404(
self.user, locale="de", slug="test-document", look_for_translation_via_parent=True
)
self.assertEqual(doc, self.de_doc)

doc = get_visible_document_or_404(
self.user, locale="de", slug="test-document-fr", look_for_translation_via_parent=True
)
self.assertEqual(doc, self.de_doc)

def test_fallback_to_parent(self):
"""Test falling back to parent document when translation doesn't exist."""
no_es_doc = ApprovedRevisionFactory(
document__locale="en-US", document__slug="no-spanish"
).document

with self.assertRaises(Http404):
get_visible_document_or_404(
self.user, locale="es", slug="no-spanish", look_for_translation_via_parent=True
)

doc = get_visible_document_or_404(
self.user,
locale="es",
slug="no-spanish",
look_for_translation_via_parent=True,
return_parent_if_no_translation=True,
)
self.assertEqual(doc, no_es_doc)

def test_nonexistent_document(self):
"""Test that we get a 404 for documents that don't exist in any locale."""
with self.assertRaises(Http404):
get_visible_document_or_404(
self.user,
locale="en-US",
slug="doesnt-exist",
look_for_translation_via_parent=True,
)

with self.assertRaises(Http404):
get_visible_document_or_404(
self.user, locale="de", slug="doesnt-exist", look_for_translation_via_parent=True
)

def test_unapproved_revision(self):
"""Test that users can't see documents without approved revisions."""
unapproved_doc = RevisionFactory(
document__locale="en-US", document__slug="unapproved", is_approved=False
).document

random_user = UserFactory()
with self.assertRaises(Http404):
get_visible_document_or_404(random_user, locale="en-US", slug="unapproved")

creator = unapproved_doc.revisions.first().creator
doc = get_visible_document_or_404(creator, locale="en-US", slug="unapproved")
self.assertEqual(doc, unapproved_doc)
53 changes: 40 additions & 13 deletions kitsune/wiki/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,26 +182,53 @@ def get_visible_document_or_404(
except Document.DoesNotExist:
pass

if (
not look_for_translation_via_parent
or not (locale := kwargs.get("locale"))
or (locale == settings.WIKI_DEFAULT_LANGUAGE)
):
# We either don't want to try to find the translation via its parent, or it doesn't
# make sense, because we're not making a locale-specific request or the locale we're
# requesting is already the default locale.
if not (locale := kwargs.get("locale")):
raise Http404

# We couldn't find a visible translation in the requested non-default locale, so let's
# see if we can find a visible translation via its parent.
slug = kwargs.get("slug")

if slug and look_for_translation_via_parent:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again many redundant comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing.

# Find documents with this slug in other locales
other_docs_qs = (
Document.objects.filter(slug=slug).exclude(locale=locale).select_related("parent")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use visible here too like above instead of filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts were that since we are looking for a path to a document through other documents, the documents we look at to move to a valid document should all be collected. We check for visibility later before presenting a final document. What if a document is restricted in one language but not in another?

Copy link
Collaborator

@akatsoulas akatsoulas Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's problematic and it shouldn't happen. The restricted visibility should be the same across all locales

)

if locale == settings.WIKI_DEFAULT_LANGUAGE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels inefficient. We have basically 2 blocks of code (if/else). Each block has the same loop and a call to translated_to. This should have been the other way. Loop once over the docs and check what you need for each one

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most probably it's cleaner to work with the existence of the parent.doc instead:

for doc in other_docs_qs:
    base_doc = doc.parent or doc
    if base_doc.locale == settings.WIKI_DEFAULT_LANGUAGE:
        if base_doc.locale == locale:
            return base_doc
        translation = base_doc.translated_to(locale, visible_for_user=user)
        if translation:
            return translation
    else:
        translation = base_doc.translated_to(locale, visible_for_user=user)
        if translation:
            return translation

# We're in default locale, look for translations in other locales
for doc in other_docs_qs:
parent_doc = doc.parent or doc

# If the parent is in the default locale, return it directly
if parent_doc.locale == settings.WIKI_DEFAULT_LANGUAGE:
return parent_doc

# Otherwise check if it has a translation in the requested locale
translation = parent_doc.translated_to(locale, visible_for_user=user)
if translation:
return translation
else:
# Looking for a non-default locale document
for doc in other_docs_qs:
# Only consider documents that are translations
if not doc.parent:
continue

# Check if this parent has a translation in our requested locale
translation = doc.parent.translated_to(locale, visible_for_user=user)
if translation:
return translation

if not look_for_translation_via_parent or locale == settings.WIKI_DEFAULT_LANGUAGE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this check is so late in the code after all the expensive operations above? This should be combined with the earlier 404.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - the second check after the loop prevents the final fallback when we're in the default language. Checking it earlier breaks the case where a user requests a document in one locale but is using a slug from another. So - check to see if we should bother finding a translation.

raise Http404

# Try to find a translation via the parent
kwargs.update(locale=settings.WIKI_DEFAULT_LANGUAGE)
parent = get_object_or_404(Document.objects.visible(user, **kwargs))

# If there's a visible translation of the parent for the requested locale, return it.
if translation := parent.translated_to(locale, visible_for_user=user):
translation = parent.translated_to(locale, visible_for_user=user)
if translation:
return translation

# Otherwise, we're left with the parent.
if return_parent_if_no_translation:
return parent

Expand Down